Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authentication, Translations, Navigation updates #20

Merged
merged 18 commits into from
Oct 6, 2023

Conversation

jim-taylor-business
Copy link
Contributor

@jim-taylor-business jim-taylor-business commented Oct 1, 2023

updates navigation with minimal styling (uses plain daisy/tailwind classes)

other changes included

  • login and authentication fully working and integrated into lemmy API calls
  • cookie handling (server and browser) for storing jwt token
  • update leptos crates generally
  • add i18n crate
  • add translation files and translations for nav
  • simple break-point layout
  • pin lemmy API crate
  • no styling on login as yet

@jim-taylor-business jim-taylor-business changed the title Navigation updates Authentication, Translations, Navigation updates Oct 3, 2023
@jim-taylor-business
Copy link
Contributor Author

@dessalines - we have come up with 2 different authentication approaches in these latest PRs

happy to go with whichever you think is best :)

@SleeplessOne1917
Copy link
Member

Some of the problems our PRs solve overlap and some don't. Some notes on your PR:

  • I'm surprised you were able to make requests from the server functions to the API without setting up proxying as as done in this PR. Attempts to do that failed for me without setting up the actix server to handle requests to the /api/* route differently.
  • Managing cookies on the client side seems like it has more errors that need to be handled than I would expect. I wonder if there's an easier way to to this.
  • When running your changes locally, can you hit the API when using localhost:1237 as the host, e.g. localhost:1237/api/v3/post/list? Other lemmy UIs are going to need to have requests to the API go through for them to work.

@jim-taylor-business
Copy link
Contributor Author

thanks for looking at the PR! i hope we can use the best bit from both our PRs.

* I'm surprised you were able to make requests from the server functions to the API without setting up proxying as as done in this PR. Attempts to do that failed for me without setting up the actix server to handle requests to the /api/* route differently.

i had no issues at all making authentication calls from server functions or content API calls to Voyager in SSR mode. Voyager also sets CORS headers to allow the same API calls direct from the browser. i'm also seeing personalized data when i set the Authorization headers, so that all seems to be working great. maybe some network peculiarities are preventing you from doing the same, or calling your own dev lemmy API.

* Managing cookies on the client side seems like it has more errors that need to be handled than I would expect. I wonder if there's an easier way to to this.

this is the first iteration of my browser/server cookie handling code. open to any suggestions to improve any code in the PR.

Rust wonderfully makes us aware of all the possibilities that emerge from our code. i have tried to allow errors to gracefully be passed up the call stack without masking the original error. i hope this means any errors can be handled at an appropriate error boundary and there will be enough detail in the error to aid debugging and useful error handling. i see there is an Issue open to define errors and handling strategy in the app. i should add any ideas in there i guess.

* When running your changes locally, can you hit the API when using localhost:1237 as the host, e.g. localhost:1237/api/v3/post/list? Other lemmy UIs are going to need to have requests to the API go through for them to work.

i have not set up my own instance as yet but i will spin up either a containerized (if one exists with the correct lemmy API) or native instance and try it out.

@SleeplessOne1917
Copy link
Member

As a heads up, I'm likely going to close this PR without merging in the future after #19 gets merged. I have 3 reasons for this:

  1. I think managing the cookies on the client side will be unnecessary, as cookies are already always sent with requests from the browser.
  2. My PR should avoid creating more than one client on the server side. While my PR uses awc as the HTTP client library, the reqwest docs also suggest creating only one client and reusing it. If I understand correctly, the api wrapper currently creates a new client every time a request is made, which is something that should be avoided if possible.
  3. I am going to experiment with a different approach to letting the browser side know that a user is logged in, as well as managing the state of the site more generally.

I will will wait a few days to see if @dessalines has any input on either of our PRs. If we hear nothing back from him after a few more days, I will merge my PR and get started on my attempt at session handling.

locales/en.json Show resolved Hide resolved
src/api/mod.rs Outdated Show resolved Hide resolved
let mut cookie = Cookie::build(path, value).finish();
let mut now = OffsetDateTime::now_utc();
now += Duration::weeks(1);
cookie.set_expires(now);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should come from the cookie / auth response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean the expiry date?

sorry i don't think i see one in the login response.

src/lib.rs Show resolved Hide resolved
src/ui/components/common/nav.rs Show resolved Hide resolved
<main class="container mx-auto">
<Routes>
<Route path="" view=HomeActivity ssr=SsrMode::Async/>
<Route path="home" view=HomeActivity ssr=SsrMode::Async/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should all be able to work with out of order streaming tho, otherwise it'll be much slower.

Copy link
Contributor Author

@jim-taylor-business jim-taylor-business Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the out of order streaming requires javascript to work which negates the benefits of fully SSR pages

can we change this when we have a full understanding of which pages and how much of those pages needs to be rendered on the server?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to. Modes are described here: https://leptos-rs.github.io/leptos/ssr/23_ssr_modes.html

@dessalines
Copy link
Member

@SleeplessOne1917 don't close this one, @jim-taylor-business fully got authentication working here, as well as a lot of other additions like translations and a fuller navbar.

Work with them to fix some of the conflicts, and figure out the optimal way to do authentication.

src/api/mod.rs Outdated Show resolved Hide resolved
@dessalines
Copy link
Member

There's a ./format.sh script in the top level dir, I'll run it for ya here.

@dessalines dessalines enabled auto-merge (squash) October 6, 2023 13:09
@dessalines dessalines merged commit 56bed13 into LemmyNet:main Oct 6, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants